Package.resolved (1)

2-2: Consider not committing Package.resolved for a library package.

For libraries, committing Package.resolved can constrain your consumers’ dependency resolution unnecessarily. If you want reproducible CI, keep it; otherwise, consider removing it and letting dependents resolve within your declared range.

Sources/PrincipleMacros/Syntax/Helpers/GlobalActorIsolationPreference.swift (1)

11-15: Add brief docs to clarify semantics (nil vs. nonisolated).

Consider documenting how this enum is intended to be used (e.g., nil at the call site meaning “unspecified,” .nonisolated meaning “explicitly nonisolated,” and .isolated(T) meaning “isolate to T”). This reduces ambiguity at integration points.

Suggested inline docs:

 public enum GlobalActorIsolationPreference: Hashable {
-
-    case nonisolated
-    case isolated(TypeSyntax)
+    /// Explicitly prefer nonisolated (e.g., the argument was present and `nil`)
+    case nonisolated
+    /// Explicitly isolate to the given global actor type (e.g., `SomeGlobalActor.self`)
+    case isolated(TypeSyntax)
 }

Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilderSettings.swift (1)

16-22: Consider documenting the new setting to avoid misuse.

A short doc comment on the property and initializer parameters will help future readers understand the nil/.nonisolated/.isolated distinctions.

Suggested diff:

-    public var globalActorIsolationPreference: GlobalActorIsolationPreference?
+    /// Global actor isolation preference extracted from parameters (if any).
+    /// - nil: no preference specified by the user; builder may apply defaults.
+    /// - .nonisolated: user explicitly prefers `nonisolated`.
+    /// - .isolated(T): user explicitly prefers isolation to the given global actor type `T`.
+    public var globalActorIsolationPreference: GlobalActorIsolationPreference?
@@
-        accessControlLevel: AccessControlLevel,
-        globalActorIsolationPreference: GlobalActorIsolationPreference? = nil
+        accessControlLevel: AccessControlLevel,
+        globalActorIsolationPreference: GlobalActorIsolationPreference? = nil
     ) {

Sources/PrincipleMacros/Syntax/Extensions/WithModifiersSyntax.swift (1)

36-41: Detail-based filtering is good; consider guarding against future tokenization changes

The detail?.detail.tokenKind match is tight and correct today. If SwiftSyntax adjusts tokenization of private(set) detail again, you’ll have a silent fallback to the primary ACL. The suggested fix above makes the resolution resilient without changing this helper. No code change required here beyond the setter fix.

Sources/PrincipleMacros/Syntax/Custom/BasicDeclSyntax.swift (1)

12-13: Adding WithAttributesSyntax is sensible; verify all BasicDeclSyntax adopters comply

Expanding BasicDeclSyntax to include WithAttributesSyntax is a good tightening of the abstraction. Ensure every site that provides any BasicDeclSyntax actually uses decls that conform to WithAttributesSyntax; otherwise you’ll get type constraint errors.

If you want, I can scan the repo for usages of any BasicDeclSyntax and list the concrete decl types to double-check conformance.

Sources/PrincipleMacros/Builders/Declarations/Types/ActorDeclBuilder.swift (1)

16-21: Minor: add a brief doc comment for discoverability

A short doc comment on typeDeclaration improves IDE discoverability and maintains parity with other builders.

     public var typeDeclaration: any TypeDeclSyntax {
-        declaration
+        // The concrete actor declaration treated as a general type declaration.
+        declaration
     }

Sources/PrincipleMacros/Builders/Declarations/Common/DeclBuilder.swift (2)

35-43: Member inheritance shortcut is fine; consider naming clarity and small guard refactor

Logic is sound. Two small readability tweaks:

• Avoid shadowing settings (outer property) with let settings = settings.accessControlLevel (rename to aclSettings).
• Precompute internalIndex once to avoid repeatedly searching Keyword.accessControlLevels if this path expands.

Optional refactor for naming clarity:

-        let settings = settings.accessControlLevel
+        let aclSettings = settings.accessControlLevel
         guard let accessControlLevel = basicDeclaration.accessControlLevel,
               let index = TokenKind.accessControlLevels.firstIndex(of: accessControlLevel.tokenKind),
-              let maxAllowedIndex = Keyword.accessControlLevels.firstIndex(of: settings.maxAllowed)
+              let maxAllowedIndex = Keyword.accessControlLevels.firstIndex(of: aclSettings.maxAllowed)
         else {
             return nil
         }
 ...
-        switch settings.inheritingDeclaration {
+        switch aclSettings.inheritingDeclaration {

---

48-57: String-interpolated AttributeSyntax is convenient; verify SwiftSyntaxBuilder isn’t required

let globalActor: AttributeSyntax = "@\(globalActor)" relies on string interpolation builders. If SwiftSyntaxBuilder is not imported in this target, this may fail to compile depending on SwiftSyntax version. If you hit that, either import SwiftSyntaxBuilder here or construct the attribute using initializers.

Otherwise, the overall isolation preference flow looks correct and the trailing space handling is consistent.

Tests/PrincipleMacrosTests/Parameters/ParameterExtractorTests.swift (3)

46-51: Drop unnecessary try if trailingClosure stops throwing

trailingClosure(withLabel:) currently has no throwing paths. If you accept the API cleanup suggested in the implementation, remove try here.

Apply this diff if you make trailingClosure non-throwing:

-        let extracted = try extractor.trailingClosure(withLabel: "operation")
+        let extracted = extractor.trailingClosure(withLabel: "operation")

---

54-59: Same here: remove try if trailingClosure becomes non-throwing

Keeps call sites tidy and signals there are no error paths.

Apply this diff if you make trailingClosure non-throwing:

-        let extracted = try extractor.trailingClosure(withLabel: "operation")
+        let extracted = extractor.trailingClosure(withLabel: "operation")

---

80-121: Add tests for AttributeSyntax initializer path

You’ve added ParameterExtractor.init(from: AttributeSyntax) but tests only cover freestanding macro expansions. Consider mirroring a couple of these tests using attributes to exercise that initializer.

Example to add (outside current hunks):

extension ParameterExtractorTests {
    private func makeExtractor(from attribute: AttributeSyntax) -> ParameterExtractor {
        ParameterExtractor(from: attribute)
    }

    @Test
    func testAttributeInit_GlobalActor_Nonisolated() throws {
        let attribute: AttributeSyntax = "@MyMacro(isolation: nil)"
        let extractor = makeExtractor(from: attribute)
        let extracted = try extractor.globalActorIsolationPreference(withLabel: "isolation")
        #expect(extracted == .nonisolated)
    }

    @Test
    func testAttributeInit_ExpressionExtraction() throws {
        let attribute: AttributeSyntax = "@MyMacro(value: Type.make())"
        let extractor = makeExtractor(from: attribute)
        let extracted = extractor.expression(withLabel: "value")
        let expected: ExprSyntax = "Type.make()"
        #expect(extracted?.description == expected.description)
    }
}

Would you like me to open a follow-up PR to add these?

Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (2)

40-47: Remove unused throws from trailingClosure(withLabel:)

There are no throwing paths in this method. Dropping throws clarifies the API and avoids unnecessary try at call sites.

Apply this diff:

-    public func trailingClosure(
-        withLabel label: TokenSyntax?
-    ) throws -> ExprSyntax? {
+    public func trailingClosure(
+        withLabel label: TokenSyntax?
+    ) -> ExprSyntax? {
         if let trailingClosure {
             return ExprSyntax(trailingClosure)
         }
         return expression(withLabel: label)
     }

Note: Update test call sites accordingly (remove try) as suggested in the tests review comments.

---

78-85: Minor simplification in .isolated detection

You can avoid re-wrapping declName and trim the base to prevent stray whitespace.

Apply this diff:

-        if let memberAccessExpression = MemberAccessExprSyntax(expression),
-           let referenceExpression = DeclReferenceExprSyntax(memberAccessExpression.declName),
-           referenceExpression.baseName.tokenKind == .keyword(.self),
-           let baseType = memberAccessExpression.base {
-            return .isolated("\(baseType)")
-        }
+        if let memberAccessExpression = MemberAccessExprSyntax(expression),
+           memberAccessExpression.declName.baseName.tokenKind == .keyword(.self),
+           let baseType = memberAccessExpression.base {
+            return .isolated("\(baseType.trimmed)")
+        }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

Sources/PrincipleMacros/Parameters/ParameterExtractor.swift (5)

21-29: AttributeSyntax initializer looks correct; consider broader argument support or explicit docs

Today you only handle .argumentList and treat everything else as “no arguments”. That’s fine for typical attribute invocations, but attributes can also carry other argument forms (e.g., balanced tokens, string-style). Either:

• Document that only labeled expression lists are supported, or
• Expand handling to cover other valid cases and throw a targeted error when present but unsupported.

Would you like me to draft an expansion that throws when unsupported argument forms are encountered so failures are explicit rather than silently treated as “no args”?

---

31-38: expression(withLabel:) is clean; consider a convenience overload for string labels

The token-based label match is robust. For ergonomics at call sites (esp. in tests), consider adding an overload that takes String? and converts it to a TokenSyntax internally.

You can add this outside the changed segment:

// Convenience overload (optional)
public func expression(withLabel label: String?) -> ExprSyntax? {
    expression(withLabel: label.map { .identifier($0) })
}

---

40-47: throws is unnecessary here; drop it to signal semantics clearly

This method never throws anymore. Keeping throws forces callers to use try with no benefit and obscures the new “nil means not found” semantics.

Apply this diff:

-    public func trailingClosure(
-        withLabel label: TokenSyntax?
-    ) throws -> ExprSyntax? {
+    public func trailingClosure(
+        withLabel label: TokenSyntax?
+    ) -> ExprSyntax? {
         if let trailingClosure {
             return ExprSyntax(trailingClosure)
         }
         return expression(withLabel: label)
     }

If you’re intentionally preserving throws for source-compatibility, a brief comment explaining that would help future readers.

---

49-65: String extraction semantics are sensible; interpolation intentionally rejected

Returning nil when the argument is missing and throwing on non-literal strings (e.g., interpolations) is a good split that mirrors common macro expectations.

Minor style nit: you mix .as(T.self) and typed-view initializers elsewhere. Consider consistently using one style for readability.

---

67-85: Global-actor isolation preference: solid coverage for nil and Type.self; trim base for robustness

The detection for nil → .nonisolated and Type.self → .isolated("Type") is correct and matches the tests. To avoid incidental whitespace in the captured type, consider trimming the base expression before interpolation.

Apply this diff:

-        if let memberAccessExpression = MemberAccessExprSyntax(expression),
+        if let memberAccessExpression = MemberAccessExprSyntax(expression),
            memberAccessExpression.declName.baseName.tokenKind == .keyword(.self),
            let baseType = memberAccessExpression.base {
-            return .isolated("\(baseType)")
+            return .isolated("\(baseType.trimmed)")
         }

Optionally, if you plan to allow MainActor without .self in the future, you could add a path for DeclReferenceExprSyntax or a TypeExprSyntax before throwing.

Do you want a follow-up to support Type without .self?

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.